Conversation
| // if event is backfillable or cached we don't need to flush the logs, because it's an event | ||
| // from the past. Otherwise we need to flush the logs to ensure they are sent on crash | ||
| if (event != null && !isBackfillable && !isCached && event.isCrashed()) { | ||
| loggerBatchProcessor.flush(options.getFlushTimeoutMillis()); |
There was a problem hiding this comment.
I'm not sure on this.
In theory we could wait for flushTimeoutMillis twice, once here and once to flush the crash.
In practice I expect this method to be instantaneous in Android, as it doesn't wait for them to be sent, and there shouldn't be many logs on Android anyway
There was a problem hiding this comment.
IMHO crashes have the highest priority, so I would avoid any extra work/wait which could cause them to be not sent. => Just flush the logs to disk with a minimal timeout.
There was a problem hiding this comment.
technically, the event is captured first, so it will be sent before the logs anyway
but yeah, let's put a minimal timeout
There was a problem hiding this comment.
I moved the capture of replay and logs after the crash event one.
But now we are waiting only for the crash to be flushed to disk, with the risk of losing replay and logs (except for those 500ms on logs)
Is it fine this way?
@romtsn wdyt?
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 85d7417 | 347.21 ms | 394.35 ms | 47.15 ms |
| 674d437 | 355.28 ms | 504.18 ms | 148.90 ms |
| 7314dbe | 437.83 ms | 505.64 ms | 67.81 ms |
| 3699cd5 | 423.60 ms | 495.52 ms | 71.92 ms |
| 17a0955 | 372.53 ms | 446.70 ms | 74.17 ms |
| ee747ae | 382.73 ms | 435.41 ms | 52.68 ms |
| ee747ae | 405.43 ms | 485.70 ms | 80.28 ms |
| b750b96 | 421.25 ms | 444.09 ms | 22.84 ms |
| ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
| ee747ae | 386.94 ms | 431.43 ms | 44.49 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 85d7417 | 1.58 MiB | 2.10 MiB | 533.44 KiB |
| 674d437 | 1.58 MiB | 2.10 MiB | 530.94 KiB |
| 7314dbe | 1.58 MiB | 2.10 MiB | 533.45 KiB |
| 3699cd5 | 1.58 MiB | 2.10 MiB | 533.45 KiB |
| 17a0955 | 1.58 MiB | 2.10 MiB | 533.20 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| b750b96 | 1.58 MiB | 2.10 MiB | 533.20 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
moved replay capture after sending the crash
stefanosiano
requested review from
adinauer,
lcian and
romtsn
as code owners
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
moved replay capture after sending the crash
Added a section for fixes and improvements in CHANGELOG.
|
Closing as this will be tackled as part of a larger initiative properly getsentry/rfcs#148 |
4 participants
📜 Description
When a crash occurs, logs are flushed with a 500 timeout millis
Moved replay capture after sending the crash
💡 Motivation and Context
We ensure to store logs to disk (or even send them) instead of losing them due to being only in memory when a crash occurs
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps